Websock support #5063

mtrudel · 2022-11-02T02:29:50Z

This is part two of #5030, adding in support for (newly renamed) WebSock facilitated upgrades. This makes Phoenix entirely agnostic to the underlying server, so long as it provides Plug & WebSock support.

The WebSock behaviour as published in 0.4.1 mirrors Phoenix.Socket.Transport nearly exactly, with a few additive changes:

c:WebSock.init/1 can now return the same set of tuples as c:WebSock.handle_in/2 et al (this set include the current {:ok, state} tuple returned today). This allows init to send messages at connection time
c:WebSock.terminate/1 defines more possible arguments, codifying a number of common termination reasons such as server shutdown or timeout. The list is described in the WebSock documentation. Given that c:Phoenix.Socket.Transport.terminate/1 has an open-ended spec currently this should be fine
Any of the callbacks which return {:push, ...} or {:reply, ...} may indicate the transmission of any type of frame
c:Phoenix.Socket.Transport.start_link/1 moves to c:Phoenix.Socket.start_link/1 and is not part of the WebSock specification

mtrudel · 2022-11-02T17:40:10Z

@josevalim this PR now stands complete for review

josevalim · 2022-11-02T18:36:40Z

lib/phoenix/socket/transport.ex

+  Of note, this behaviour is a superset of the `WebSock` behaviour, and
+  as such all implementations of `Phoenix.Socket.Transport` are also
+  implementations of `WebSock`. It is not possible to note this in code
+  without raising warnings about conflicting implementations.


Ah... I see now. Phoenix.Socket.Transport in Phoenix is generic. WebSock is an implementation of Phoenix.Socket.Transport for WebSockets + a generic abstraction for adapters. So now that I think about it, I don't think we should list WebSock here.

I.e. we should just remove this whole paragraph and instead say that WebSock implements Phoenix.Socket.Transport.

I.e. we should just remove this whole paragraph and instead say that WebSock implements Phoenix.Socket.Transport.

Do you mean that we should say that here, just in a differently worded comment, or that WebSock should actually @impl Phoenix.Socket.Transport over in the WebSock project?

FWIW I think the Phoenix.Socket.Transport API itself is 100% fine as a WebSocket abstraction (as we've talked about at length on previous PRs). This is more a question of 'which behaviour subsumes which behaviour' and being able to express that at the limits of the language's behaviour mechanism.

Right, the thing is that Phoenix.Socket.Transport is meant to be transport agnostic, so it doesn't make sense to say it implements WebSock.

At the same time, we don't want WebSock to depend on Phoenix, because people can use it without Phoenix.

So I think we should just informally says in WebSocket that it implements Phoenix.Socket.Transport and that's it. WDYT?

This is also the reason why I think it should exist in the Phoenix org (I was originally considering it should be Plug but I think it is more aligned to Phoenix).

Happy to note it at both ends (in fact, I'm pretty sure I already do note the similarity in the WebSock docs, at least as a source of inspiration). My main worry (speaking from experience as I pieced together an understanding of all of Phoenix's internals) is that without some sort of note here, it's not at all obvious how a fundamental part of Phoenix's WebSocket implementation works. Maybe the note could instead live in Phoenix.Transport.WebSocket, as a code comment near the WebSock.upgrade/4 call?

I actually think that's better, TBH. As I've mentioned before, someone trying to understand how WebSockets work in Phoenix is going to check out Phoenix.Transports.WebSocket first (just given the name), and so as many breadcrumbs as possible should be left there. It also keeps Phoenix.Socket.Transport agnostic.

As an aside, the ability for behaviours to compose other behaviours is something I've wished for on multiple occasions (a concrete example is ThousandIsland.Handler wanting to be a superset of GenServer). Has this been proposed before / are there plans for it / is it something you'd entertain a PR for in the language?

Maybe the note could instead live in Phoenix.Transport.WebSocket, as a code comment near the WebSock.upgrade/4 call?

Agreed!

Has this been proposed before / are there plans for it / is it something you'd entertain a PR for in the language?

Nothing we can do. Behaviours are somewhat tied to Dialyzer and Dialyzer will complain anyway. :(

josevalim · 2022-11-02T18:40:19Z

Yeah, this is as good as it gets. :) I will talk to @chrismccord about his feelings on this and the ownership. Would you be ok with moving WebSock to the Phoenix org? You'd still remain as admin but we would be around to help/backup.

mtrudel · 2022-11-02T18:52:09Z

I have no problem with that! I've actually been thinking about the longer-term plans for the entire Thousand Island / Bandit / WebSock stack and how there should be an elixir-networking org to host all of that (a bit of an overlap with elixir-plug; maybe it would be a better home)?

In any case, having been bit firsthand by 'the bus effect' of maintainers just disappearing I'm more than happy to have stuff live in an org.

mtrudel · 2022-11-02T19:29:06Z

Docs updated, merge conflicts resolved.

I did a top level comment in Phoenix.Transport.WebSocket that outlines the overall lifecycle; it's the comment that I wish existed when I started exploring Phoenix, and in the first place anyone would look :).

Let's wait to hear @chrismccord 's take on the matter, but I'm happy to move the WebSock project to any org you'd like

mtrudel · 2022-11-03T15:20:41Z

I'm running into an issue with Hex dependencies now that WebSock depends on Cowboy. I'd originally conceived of WebSock as being strictly behaviour, but now that it's an actual abstraction on top of Cowboy & Bandit there's an inverted dependency that makes it so Bandit ends up having a transitive dependency on Cowboy (not great). It also makes WebSock's handling of Bandit upgrades untestable since there's a circular dependency (this isn't a big deal since the upgrade is a one-line passthrough to Bandit's native WebSock support).

To fix this I need Bandit to be able to depend on just the WebSock behaviour and not pull in any of the adapter dependencies. Similar to the split between plug and plug_cowboy, I think I'm going to end up cutting a websock_adapter package to contain the actual adapters separate from the base WebSock behaviour (this is what Phoenix will end up depending on). No new code in any of this, just a bit of package juggling.

Let's hold off on merging this until I get that lined up. Should be later today (EDT).

josevalim · 2022-11-03T15:35:22Z

@mtrudel why does Bandit depend on WebSock? Doesn't Bandit expose a more general WebSockets API anyway?

mtrudel · 2022-11-03T15:40:23Z

No - the whole intent of WebSock was to be that general WebSocket API (and in my mind, it still mostly is). If you look at WebSock's 'adapter' to Bandit, it just wires Bandit directly into the handler (see https://github.com/mtrudel/websock/blob/main/lib/websock.ex#L167-L168). Running on Bandit you'll never see any WebSockAdapter frames in a stack trace, because there's no code there at all.

The discussion we had previously that ended up reshaping the WebSock API in the mold of Transport was more about the tradeoffs of seeing the API from the perspective of a server implementor (what it was originally) or the perspective of a framework implementor (what it is now). I still think it stands as a generalized WebSocket API, just one that makes more sense from the framework side of things now.

Splitting just the behaviour out into its own library allows it to continue being a generalized API. The implementations on top of Bandit or Cowboy stand apart from that.

Before I get too far into this, do you have a better idea? I'm all ears for a different approach here.

josevalim · 2022-11-03T17:07:51Z

What if:

Let's make sock be the contract. It doesn't mention anything about websockets
websock is the implementation of the sock contract and specific upgrades for websockets

?

mtrudel · 2022-11-03T17:22:41Z

That's more or less the split that I have in mind, just that the base behaviour doesn't try to be generic (it's more specific than a generalized socket interface, since it knows about frame types. It doesn't really make sense to not be WebSocket specific).

The split is actually turning out to be pretty straightforward, and enables everything I was hoping to in terms of declaring conformance and testability. Should be up shortly.

mtrudel · 2022-11-03T18:56:44Z

Updated (see d7730a2 for the relevant change to this PR; it's trivial).

You can see WebSock/WebSockAdapter split at:

(as before, happy for both project to move under the Phoenix (or any other) umbrella).

Tests are green up and down the whole stack; everything is once again ready for review!

josevalim · 2022-11-03T19:50:02Z

Hi @mtrudel! One question: is there a particular reason why you preferred to not go with sock for spec and websock for the implementations? Is it because the current contract has websock specific functionality?

mtrudel · 2022-11-03T20:52:28Z

Yep - the sock -> websock rename was, to my mind, a literal rename. Nothing changed semantically & I never intended the rename to convey anything more than having a less ambiguous name for the same unchanged concept.

I've always conceived the WebSock (née Sock) API to be a WebSocket abstraction and nothing more (specifically, I've never thought of it as a generalization over other transports such as long polling). This is in contrast to Transport, which intends to be generic over all transports at the expense of fidelity. The WebSock spec is pretty specific to WebSockets (insofar as it knows about frame types and the specifics of the overall lifecycle).

The tl;dr here is that I think this difference of opinion (if I can even call it that) comes from the differing perspectives of a server-first or framework-first frame of reference. From my server-first perspective, I set out for WebSock to be the solution to "how can I generalize different servers' WebSocket implementation?" whereas from a framework-first perspective it may have been seen as trying to solve "how can I generalize 'sockets' across different transports?". WebSock is not and never has been trying to be the latter of those two questions [1], and to the extent that it manages to fill that role today it's partly 'happy accident' and partly from having conceded on some of the higher fidelity aspects of WebSockets in the API (connection close codes are a good example of this). All in all I'm very happy with how it turned out!

You can see some of this change in perspective by comparing what's here now to the shape of my early PRs on this work, where I was approaching the abstraction very much from the perspective of 'Plug for WebSockets' rather than trying to be anything more general. From a 'server-first' perspective I still think that model is/was the right approach, but shipping (and building consensus!) is more important than purity! An API that isn't adopted isn't worth anything.

The split between the WebSock behaviour and the WebSockAdapter adapter lib reflected here is purely a matter of necessity, to allow code both 'above' and 'below' the adapter layer to be able to interact with the spec (you'll note that Bandit depends on websock, while Phoenix depends on websock_adapter (and of course then transitively on websock)).

A bit longwinded, but I hope this helps explain the thinking here?

[1] Having a library that generalized over multiple transports could be a really interesting thing to evolve towards in the future!

mtrudel · 2022-11-03T20:53:33Z

Updated to WebSock 0.4.3 which provides support for return values to specify multiple frames to send:

{:push, [{:text, "TEXT"}, {:binary, "BINARY"}], state}

This is a strictly additive change to the existing contract

Gazler · 2022-11-04T12:04:58Z

Thanks for all the hard work @mtrudel I just tried out this PR on master and can confirm that cowboy and bandit are both working with a fresh liveview project, it was seamless!

I've reviewed this PR as well as websock and websock_adapter and it all looks good to me.

josevalim · 2022-11-04T12:19:20Z

@mtrudel, can you please add me as admin to both projects so I transfer them to the Phoenix org and merge this?

mtrudel · 2022-11-04T12:24:17Z

@mtrudel, can you please add me as admin to both projects so I transfer them to the Phoenix org and merge this?

Done! You should have two invites pending. Not sure if you'll be able to transfer them since they're in a personal account and not an org. Let's see.

mtrudel · 2022-11-04T12:25:24Z

I'll have to add you as an admin to the hex projects too. Will do that later this morning!

mtrudel · 2022-11-04T13:25:29Z

@josevalim I added you to the hex packages. In terms of the repos, it looks like I need to be the one to do the transfer, and I can only do so to another personal account or to an org where I have permission to create repos. Probably easiest if I just transfer it to your josevalim personal account and you can bounce it over to the org from there. Will do so now.

mtrudel · 2022-11-04T13:28:53Z

@josevalim done. You should now have two invitations to transfer the repos to your personal account, from where you can immediately transfer them to phoenixframework. Note that you'll probably need to add me in the org, as I don't think my permissions will transfer all the way through.

josevalim · 2022-11-04T13:39:38Z

lib/phoenix/socket/transport.ex

-
-  means `child_spec([shutdown: 5000])` will be invoked.
-  """
-  @callback child_spec(keyword) :: :supervisor.child_spec()


Why was this moved to Phoenix.Socket? I don't think it is actually part of Phoenix.Socket, because the user does not need to define a child_spec for Phoenix.Socket. :)

(Note that we've talked about this before, but I suppose never came to a conclusion on it)

The actual concrete implementation of this callback hasn't changed - it's still used into the Socket at https://github.com/mtrudel/phoenix/blob/af00565585bdbc5644f9ed0a1077d52d87ac3f0f/lib/phoenix/socket.ex#L309-L311. The user does not and never has needed to define a child_spec themselves.

The only thing that changed here is which behaviour this is part of. The scope of this callback based on its docs makes this seem like a better home for it. Not strictly blocking, though.

It should be part of Phoenix.Socket.Transport because someone can implement Phoenix.Socket.Transport so they bypass the whole channel machinery and I believe that, without this callback, it won't work.

josevalim

One comment and I think we can ship this.

….6.0)

…I started

josevalim · 2022-11-04T14:02:36Z

💚 💙 💜 💛 ❤️

mtrudel · 2022-11-04T14:12:17Z

A M A Z I N G

Now that this is finalized I've published the last of the Bandit 0.5 series (0.5.10), which wraps up WebSocket and Phoenix support! Being able to say 'Bandit works with Phoenix 1.7, no strings attached' is huge for adoption. Thank you all so much for helping to make it happen.

I've got a number of follow-on PRs relating to completing the transition to Cowboy being 'one among many' possible servers for Phoenix, but they can & should wait until after 1.7 is out and Bandit's integration has had some time to bake in. They're all doc & generator related, nothing core.

josevalim · 2022-11-04T14:14:25Z

Awesome work @mtrudel! We will reap the benefits of these changes for a long time!

mtrudel mentioned this pull request Nov 2, 2022

Allow Phoenix to provide generic WebSocket support #5003

Closed

mtrudel force-pushed the websock_support branch 2 times, most recently from 67ab9dc to e6818e3 Compare November 2, 2022 17:33

josevalim reviewed Nov 2, 2022

View reviewed changes

mtrudel force-pushed the websock_support branch from e6818e3 to 2770c1b Compare November 2, 2022 19:27

mtrudel force-pushed the websock_support branch from 2770c1b to 3444924 Compare November 3, 2022 01:46

mtrudel force-pushed the websock_support branch from 336de8d to d7730a2 Compare November 3, 2022 18:58

mtrudel force-pushed the websock_support branch from 2eaea1e to af00565 Compare November 3, 2022 20:41

mtrudel mentioned this pull request Nov 4, 2022

Change websock directory to websock_adapter phoenixframework/websock_adapter#1

Merged

josevalim reviewed Nov 4, 2022

View reviewed changes

mtrudel added 7 commits November 4, 2022 09:53

Use WebSock upgrade functionality

2cf9276

Remove Cowboy2Handler

7ef6092

Update integration test lockfile (also grab republished plug_cowboy 2…

2bd5a39

….6.0)

Version bump of websock

6e4d04d

Add docs to Phoenix.Transports.WebSocket that I wish were there when …

86ee9e3

…I started

Use WebSockAdapter lib instead of WebSock

5cdd1fc

Update to websock 0.4.3

e070f26

mtrudel force-pushed the websock_support branch from af00565 to e070f26 Compare November 4, 2022 13:55

josevalim merged commit 623ea99 into phoenixframework:master Nov 4, 2022

mtrudel deleted the websock_support branch November 4, 2022 14:03

mtrudel mentioned this pull request Jan 31, 2023

LiveView + Bandit doesn't handle server restarts gracefully phoenixframework/phoenix_live_view#2421

Closed

Websock support #5063

Websock support #5063

Conversation

mtrudel commented Nov 2, 2022 • edited

mtrudel commented Nov 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrudel Nov 2, 2022 • edited

Choose a reason for hiding this comment

mtrudel Nov 2, 2022 • edited

Choose a reason for hiding this comment

josevalim Nov 2, 2022 • edited

Choose a reason for hiding this comment

josevalim commented Nov 2, 2022

mtrudel commented Nov 2, 2022

mtrudel commented Nov 2, 2022 • edited

mtrudel commented Nov 3, 2022 • edited

josevalim commented Nov 3, 2022 • edited

mtrudel commented Nov 3, 2022 • edited

josevalim commented Nov 3, 2022

mtrudel commented Nov 3, 2022 • edited

mtrudel commented Nov 3, 2022 • edited

josevalim commented Nov 3, 2022

mtrudel commented Nov 3, 2022 • edited

mtrudel commented Nov 3, 2022 • edited

Gazler commented Nov 4, 2022

josevalim commented Nov 4, 2022

mtrudel commented Nov 4, 2022

mtrudel commented Nov 4, 2022

mtrudel commented Nov 4, 2022 • edited

mtrudel commented Nov 4, 2022 • edited

Choose a reason for hiding this comment

mtrudel Nov 4, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josevalim left a comment

Choose a reason for hiding this comment

josevalim commented Nov 4, 2022

mtrudel commented Nov 4, 2022 • edited

josevalim commented Nov 4, 2022

mtrudel commented Nov 2, 2022 •

edited

mtrudel Nov 2, 2022 •

edited

mtrudel Nov 2, 2022 •

edited

josevalim Nov 2, 2022 •

edited

mtrudel commented Nov 2, 2022 •

edited

mtrudel commented Nov 3, 2022 •

edited

josevalim commented Nov 3, 2022 •

edited

mtrudel commented Nov 3, 2022 •

edited

mtrudel commented Nov 3, 2022 •

edited

mtrudel commented Nov 3, 2022 •

edited

mtrudel commented Nov 3, 2022 •

edited

mtrudel commented Nov 3, 2022 •

edited

mtrudel commented Nov 4, 2022 •

edited

mtrudel commented Nov 4, 2022 •

edited

mtrudel Nov 4, 2022 •

edited

mtrudel commented Nov 4, 2022 •

edited